Skip to content

[FC- 0049] feat: Features to enable import/export courses#172

Merged
bradenmacdonald merged 10 commits intoopenedx:mainfrom
open-craft:chris/FAL-3604-import-export-courses
Mar 28, 2024
Merged

[FC- 0049] feat: Features to enable import/export courses#172
bradenmacdonald merged 10 commits intoopenedx:mainfrom
open-craft:chris/FAL-3604-import-export-courses

Conversation

@ChrisChV
Copy link
Contributor

@ChrisChV ChrisChV commented Mar 22, 2024 •
edited
Loading

Description

  • Rename object_tag._name to object_tag._export_id
  • Refactor all code about taxonomy._name
  • Update resync object tags to update the taxonomy from _export_id
  • Update tag_object to allow create object_id with invalid tags and taxonomies
  • Update Language taxonomy export_id
  • Update avoid create tags that contains the tags csv separator: In some taxonomies (Lightcast Open Skills Taxonomy) there are tags that contains ,. This character is used as separator on export tags, so causes issues. This separator will change to ;, so we need to avoid to create tags with ;.

Support information

Testing instructions

  • Check that tests cover the new features.
  • Verify the new export_id of the Language taxonomy. On django shell:
from openedx_tagging.core.tagging.models.base import Taxonomy
lang = Taxonomy.objects.get(id=-1)
lang.export_id

@ChrisChV
feat: Features to enable import/export courses
0b99842 
- Rename taxonomy._name to taxonomy._export_id
- Refactor all code about taxonomy._name
- Update resync object tags to update the taxonomy from _export_id
- Update tag_object to allow create object_id with invalid tags and taxonomies
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 22, 2024 •
edited
Loading

Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 22, 2024
@ChrisChV ChrisChV changed the title feat: Features to enable import/export courses [ FC- 0049] feat: Features to enable import/export courses Mar 22, 2024
@ChrisChV ChrisChV marked this pull request as draft March 22, 2024 22:32
@ChrisChV ChrisChV changed the title [ FC- 0049] feat: Features to enable import/export courses [FC- 0049] feat: Features to enable import/export courses Mar 25, 2024
@ChrisChV ChrisChV marked this pull request as ready for review March 25, 2024 20:35
@ChrisChV ChrisChV mentioned this pull request Mar 26, 2024
Merged
1 task
bradenmacdonald
bradenmacdonald reviewed Mar 26, 2024
View reviewed changes
rpenido
rpenido approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@rpenido rpenido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @ChrisChV ! 👍

  • I tested this using the Testing Instructions from openedx/openedx-platform#34356
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@ChrisChV
Copy link
Contributor Author

ChrisChV commented Mar 27, 2024

Thanks @rpenido. I found an issue exporting tags that contains ,. I fixed that issue here c85cfc8

Update avoid create tags that contains the tags csv separator: In some taxonomies (Lightcast Open Skills Taxonomy) there are tags that contains ,. This character is used as separator on export tags, so causes issues. This separator will change to ;, so we need to avoid to create tags with ;.

Could you review that commit? @bradenmacdonald After that, it would be ready for your review.

@rpenido
Copy link
Contributor

rpenido commented Mar 27, 2024

Thanks @rpenido. I found an issue exporting tags that contains ,. I fixed that issue here c85cfc8

Update avoid create tags that contains the tags csv separator: In some taxonomies (Lightcast Open Skills Taxonomy) there are tags that contains ,. This character is used as separator on export tags, so causes issues. This separator will change to ;, so we need to avoid to create tags with ;.

Could you review that commit? @bradenmacdonald After that, it would be ready for your review.

Done @ChrisChV! I tested it again and is working fine.

bradenmacdonald
bradenmacdonald requested changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one request about how we handle reserved characters.

@bradenmacdonald bradenmacdonald merged commit a800b68 into openedx:main Mar 28, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 28, 2024

@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald bradenmacdonald deleted the chris/FAL-3604-import-export-courses branch March 28, 2024 21:35
@ormsbee ormsbee mentioned this pull request Mar 24, 2025
Closed
@ormsbee ormsbee mentioned this pull request Sep 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bradenmacdonald bradenmacdonald bradenmacdonald approved these changes

+1 more reviewer

@rpenido rpenido rpenido approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ChrisChV @openedx-webhooks @rpenido @bradenmacdonald